Conversation
… serialization and parsing from `ducrs`, update Turborepo build dependencies
…ric element to Model element
Jorgedanisc
There was a problem hiding this comment.
Pull request overview
This PR updates the DUC FlatBuffers schema and all language bindings (Rust/TS/Python) to introduce a new 3D model element (DucModelElement), add document/PDF grid layout config (DocumentGridConfig), and extend PDF rendering to support block instance duplication arrays during streaming.
Changes:
- Add
DocumentGridConfig(+DOCUMENT_GRID_ALIGN_ITEMS) and wire it intoDucPdfElementandDucDocElementacross schema + Rust/TS/Python bindings. - Introduce
DucModelElementin schema + bindings, and update TS element creation/serialization/parsing. - Add duplication-array rendering support in
duc2pdf(streaming + scaling), plus a new crop test fixture.
Reviewed changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Adjust Turbo task dependencies to include ducrs#build for ducpdf#build. |
| schema/duc.fbs | Adds DocumentGridConfig, DOCUMENT_GRID_ALIGN_ITEMS, and DucModelElement; deprecates some older schema parts. |
| packages/ducrs/src/types.rs | Adds Rust types for grid config + model element; adjusts some existing types. |
| packages/ducrs/src/serialize.rs | Serializes new grid config fields and model element into FlatBuffers. |
| packages/ducrs/src/parse.rs | Parses new grid config fields and model element; adds legacy fallback for binary JSON decoding. |
| packages/ducrs/src/flatbuffers/duc_generated.rs | Updates generated Rust FlatBuffers bindings for new schema entities. |
| packages/ducpy/src/ducpy/serialize.py | Adds Python FlatBuffers serialization for grid config + model element. |
| packages/ducpy/src/ducpy/parse.py | Adds Python FlatBuffers parsing for grid config + model element. |
| packages/ducpy/src/ducpy/classes/ElementsClass.py | Adds Python dataclasses for DocumentGridConfig and DucModelElement; updates element union. |
| packages/ducpy/src/ducpy/Duc/Element.py | Updates Element enum with DucModelElement. |
| packages/ducpy/src/ducpy/Duc/DucPdfElement.py | Adds FlatBuffers accessor + builder slot for grid_config. |
| packages/ducpy/src/ducpy/Duc/DucModelElement.py | New generated FlatBuffers Python type for DucModelElement. |
| packages/ducpy/src/ducpy/Duc/DucDocElement.py | Adds FlatBuffers accessors + builder slots for grid_config and file_id. |
| packages/ducpy/src/ducpy/Duc/DocumentGridConfig.py | New generated FlatBuffers Python type for DocumentGridConfig. |
| packages/ducpy/src/ducpy/Duc/DOCUMENT_GRID_ALIGN_ITEMS.py | New generated FlatBuffers Python enum for grid alignment. |
| packages/ducpdf/tests/crop.test.ts | Adds test coverage using a new multi-block fixture. |
| packages/ducpdf/src/duc2pdf/src/streaming/stream_elements.rs | Implements duplication-array streaming offsets; threads instance map through streamer. |
| packages/ducpdf/src/duc2pdf/src/scaling.rs | Scales duplication-array spacings when scaling exported data. |
| packages/ducpdf/src/duc2pdf/src/lib.rs | Adds WASM logger initialization entrypoint. |
| packages/ducpdf/src/duc2pdf/src/builder.rs | Builds a block_instances lookup map; updates bounds intersection logic to include duplications. |
| packages/ducpdf/src/duc2pdf/Cargo.toml | Adds log + console_log dependencies. |
| packages/ducjs/src/utils/elements/newElement.ts | Adds grid config defaults; switches “parametric” constructor to emit model element. |
| packages/ducjs/src/types/elements/typeChecks.ts | Updates element type validation to accept model. |
| packages/ducjs/src/types/elements/index.ts | Adds DocumentGridConfig + DucModelElement TS types; updates doc/pdf element fields accordingly. |
| packages/ducjs/src/serialize.ts | Serializes grid config + model element; adjusts PDF/doc fileId serialization optionality. |
| packages/ducjs/src/restore/restoreElements.ts | Restores gridConfig + fileId for doc/pdf; adds restore path for model. |
| packages/ducjs/src/restore/restoreDataState.ts | Adjusts block instance restoration to be scope-aware for duplication spacing precision values. |
| packages/ducjs/src/parse.ts | Adds parsing for grid config + model element; updates element union dispatch. |
| packages/ducjs/src/flatbuffers/duc/element.ts | Updates generated TS FlatBuffers union helpers to include DucModelElement. |
| packages/ducjs/src/flatbuffers/duc/duc-pdf-element.ts | Updates generated TS FlatBuffers type to include gridConfig. |
| packages/ducjs/src/flatbuffers/duc/duc-model-element.ts | New generated TS FlatBuffers type for DucModelElement. |
| packages/ducjs/src/flatbuffers/duc/duc-doc-element.ts | Updates generated TS FlatBuffers type to include gridConfig + fileId. |
| packages/ducjs/src/flatbuffers/duc/document-grid-config.ts | New generated TS FlatBuffers type for DocumentGridConfig. |
| packages/ducjs/src/flatbuffers/duc/document-grid-align-items.ts | New generated TS FlatBuffers enum for grid alignment. |
| packages/ducjs/src/flatbuffers/duc.ts | Re-exports new generated TS FlatBuffers types/enums. |
| packages/ducdxf/.gitignore | Ignores Python egg-info artifacts. |
| assets/testing/duc-files/multiple_blocks.duc | Adds new test fixture used by ducpdf crop tests. |
| Cargo.lock | Adds lock entries for log and console_log. |
| .documentation/SchemaUpdates.md | Minor clarification for checking staged schema changes. |
Comments suppressed due to low confidence (3)
packages/ducjs/src/parse.ts:1283
- Binary parsing no longer handles
ElementUnion.DucParametricElementeven though the FlatBuffers union still includes it (deprecated). This will cause older.ducfiles containing parametric elements to be dropped (null). Consider adding aDucParametricElementcase (mapping it to the newmodelrepresentation or preserving it) to maintain backward compatibility.
break;
case ElementUnion.DucDocElement:
element = wrapper.element(new DucDocElementFb());
break;
case ElementUnion.DucModelElement:
element = wrapper.element(new DucModelElementFb());
break;
default:
return null;
}
packages/ducjs/src/restore/restoreElements.ts:997
- The restore path no longer accepts elements with
type: "parametric". If older JSON exports still contain parametric elements, they will fail validation/restoration. Consider supportingparametricas an alias (e.g., restoring intomodel) or keeping the legacy case until a migration is in place.
case "model": {
const modelElement = element as DucModelElement;
return restoreElementWithProperties(
modelElement,
{
source: isValidString(modelElement.source),
fileIds: modelElement.fileIds || [],
svgPath: modelElement.svgPath || null,
},
localState,
globalState
);
}
packages/ducjs/src/types/elements/typeChecks.ts:276
isDucElementno longer treats"parametric"as a valid element type. This can break restoration/validation of older JSON data that still contains parametric elements. Consider temporarily allowing"parametric"(possibly mapping it to"model") for backward compatibility.
switch (type) {
case "text":
case "polygon":
case "rectangle":
case "embeddable":
case "ellipse":
case "arrow":
case "freedraw":
case "line":
case "frame":
case "image":
case "table":
case "dimension":
case "leader":
case "doc":
case "selection":
case "model":
case "featurecontrolframe":
case "viewport":
case "plot":
case "xray":
case "pdf":
case "mermaid": {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const newParametricElement = (currentScope: Scope, opts: ElementConstructorOpts): NonDeleted<DucModelElement> => ({ | ||
| source: "", | ||
| svgPath: null, | ||
| fileIds: [], | ||
| ..._newElementBase<DucModelElement>("model", currentScope, opts), | ||
| type: 'model', | ||
| }); |
There was a problem hiding this comment.
newParametricElement now constructs a DucModelElement with type: 'model'. Keeping the old function name is likely to confuse callers and makes the API misleading; consider renaming to newModelElement (and optionally keeping newParametricElement as a deprecated alias).
| * - **DONT CHANGE FIELD TYPES**: Modifying a field's type can lead to incompatibilities. If necessary, add a new field with the desired type and depreciate the old one. | ||
| * - **BE CAREFUL WITH DEFAULT VALUES**: Changing default values can cause inconsistencies when reading older data. It's best to set appropriate defaults initially and avoid altering them. | ||
| * - **STRING or TABLE CANNOT FALLBACK TO NULL**: these fields cannot be null. They already have a default null value. | ||
| * - **DEFINE JSON FIELDS AS UBYTE ARRAY**: On our architecture of the fbs file, we always store JSON as a zlib compressed binary ([ubyte]) to save space and improve performance.** |
There was a problem hiding this comment.
There’s an extra trailing ** at the end of this bullet, which breaks the intended markdown formatting in the schema header comment. Remove the stray ** (and consider fixing the earlier “depreciate” -> “deprecate” typo if you touch that line).
| #[inline] | ||
| pub fn add_grid_config(&mut self, grid_config: flatbuffers::WIPOffset<DocumentGridConfig<'b >>) { | ||
| self.fbb_.push_slot_always::<flatbuffers::WIPOffset<DocumentGridConfig>>(DucDocElement::VT_GRID_CONFIG, grid_config); |
There was a problem hiding this comment.
DucDocElementBuilder::add_grid_config has an invalid type (DocumentGridConfig<'b >>) which will not compile. Fix the generated signature to use a valid lifetime parameter (e.g., DocumentGridConfig<'b>) and ensure the generator doesn’t emit the extra >.
| @@ -363,6 +462,7 @@ impl ElementStreamer { | |||
| hatching_manager: &mut HatchingManager, | |||
| pdf_embedder: &mut PdfEmbedder, | |||
| image_manager: &mut ImageManager, | |||
| duplication_offset: Option<(f64, f64)>, | |||
| ) -> ConversionResult<Vec<Operation>> { | |||
| let mut operations = Vec::new(); | |||
| let is_plot_mode = matches!(self.current_mode, StreamMode::Plot); | |||
| @@ -414,6 +514,24 @@ impl ElementStreamer { | |||
| operations.extend(transform_ops); | |||
| } | |||
|
|
|||
| // Apply duplication offset AFTER the element's own transform so it is not rotated | |||
| // or scaled by the element transform. PDF Y axis is inverted, so negate Y. | |||
| if let Some((x_off, y_off)) = duplication_offset { | |||
| if x_off != 0.0 || y_off != 0.0 { | |||
| operations.push(Operation::new( | |||
| "cm", | |||
| vec![ | |||
| Object::Real(1.0), | |||
| Object::Real(0.0), | |||
| Object::Real(0.0), | |||
| Object::Real(1.0), | |||
| Object::Real(x_off as f32), | |||
| Object::Real((-y_off) as f32), | |||
| ], | |||
| )); | |||
There was a problem hiding this comment.
The duplication offset is applied via cm after the element transform, but in PDF the subsequent cm is post-multiplied by the current CTM, so the offset will still be affected by the element’s rotation/scale. To keep duplication offsets in page coordinates, apply the duplication translation before the element transform (or incorporate the offset into the translation terms when building the element transform matrix).
| // Special handling: PDF elements - do not apply styles to avoid affecting embedded content | ||
| let styles = self.style_resolver.resolve_styles(element, None); | ||
|
|
There was a problem hiding this comment.
This file now relies on duc::types::DucElementEnum, which includes DucModelElement, but stream_element_with_resources still matches element variants without handling DucModelElement. That will make the match non-exhaustive and fail to compile (or silently skip rendering if you add a wildcard). Add an explicit DucModelElement arm (even if it’s currently treated as WIP/ignored).
| pub fn add_grid_config(&mut self, grid_config: flatbuffers::WIPOffset<DocumentGridConfig<'b >>) { | ||
| self.fbb_.push_slot_always::<flatbuffers::WIPOffset<DocumentGridConfig>>(DucPdfElement::VT_GRID_CONFIG, grid_config); | ||
| } |
There was a problem hiding this comment.
DucPdfElementBuilder::add_grid_config has an invalid type (DocumentGridConfig<'b >>) which will not compile. This looks like a generation/formatting issue; it should use a valid lifetime parameter (e.g., DocumentGridConfig<'b>) consistent with the other builder methods.
| DucElementEnum::DucFeatureControlFrameElement(f) => (f.base.width, f.base.height), | ||
| DucElementEnum::DucParametricElement(p) => (p.base.width, p.base.height), | ||
| DucElementEnum::DucPdfElement(p) => (p.base.width, p.base.height), | ||
| DucElementEnum::DucMermaidElement(m) => (m.base.width, m.base.height), |
There was a problem hiding this comment.
get_element_duplication_offsets matches on DucElementEnum but doesn’t handle DucModelElement. Since the enum now includes that variant, this match will be non-exhaustive and fail to compile. Add a DucModelElement arm (and decide which dimensions to use, likely base.width/base.height).
| DucElementEnum::DucMermaidElement(m) => (m.base.width, m.base.height), | |
| DucElementEnum::DucMermaidElement(m) => (m.base.width, m.base.height), | |
| DucElementEnum::DucModelElement(m) => (m.base.width, m.base.height), |
| // Initialize logger for WASM | ||
| #[cfg(target_arch = "wasm32")] | ||
| #[wasm_bindgen(start)] | ||
| pub fn init_logger() { | ||
| console_log::init_with_level(log::Level::Info).expect("Failed to initialize logger"); | ||
| } |
There was a problem hiding this comment.
init_logger uses expect(...) on logger init. In wasm contexts (or tests) it’s easy to initialize logging more than once, which would panic here. Consider handling the error non-fatally (e.g., ignore SetLoggerError / already-initialized) so conversion doesn’t fail due to logging setup.
fix: Add DucModelElement support and remove BlockManager from PDF gen…
|
🎉 This PR is included in version 2.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.4.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.